Skip to content

Let an agent account for a finding GitHub cannot close#48

Open
kristofferR wants to merge 12 commits into
mainfrom
feat/dismiss-unresolvable-finding
Open

Let an agent account for a finding GitHub cannot close#48
kristofferR wants to merge 12 commits into
mainfrom
feat/dismiss-unresolvable-finding

Conversation

@kristofferR

@kristofferR kristofferR commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Roadmap item D — the #4 annoyance in the transcript sweep, and the one with the most damning
observed end state: "No review was ever requested for 7a16eeb." Four rounds, one PR, no review.

The deadlock

crq resolve and crq decline both act on a review thread. These findings have none:

  • a review-body finding (CodeRabbit's outside-diff section),
  • a review_skipped notice,
  • any collapsed body remark crq parses but GitHub never threaded.

Drain-first says a round may not start while findings for the current head are undrained. So the
round cannot start because the finding is undrained, and the finding cannot drain because nothing
can act on it. crq next returns fix forever and no review is ever requested for the head.

crq decline was already tightened to resolve by default for exactly this reason (issue #8). That
fixed the threaded case and left this one untouched.

The lever

crq dismiss <repo> <pr> <finding-id>... --reason "why this is set aside"

Dismissed findings are withheld from the action, so the round moves on. crq next reports
dismissed: N — a set-aside finding must not look like one that was never reported.

Three choices worth arguing about:

The reason is stored, not just demanded. A dismissal that discards its justification is not
auditable, so Round.Dismissed maps ID → reason rather than being a list of IDs.

Finding IDs, not node IDs. .findings[].id is a hash of the finding's own content, so unlike a
thread ID it is not globally unique — the repo and PR genuinely identify something here and are
required.

Scoped to the current head. The same content yields the same ID, so a dismissal that outlived
its head would silently swallow the finding when the next reviewer reports it again. Superseding the
round drops it, which is the rule body findings already follow: the current reviewer must report it
again.

And dismissing enqueues the PR when crq is not yet tracking the current head. That is deliberate:
no-round-for-the-head is the deadlock's own signature, so a dismissal with nowhere to live would
change nothing.

Verification

TestDismissEndsTheUnresolvableFindingDeadlock walks the whole shape: a body finding lands with no
thread, fix repeats across two calls, dismissing ends it, the finding is withheld, dismissed is
1, nothing was posted by the dismissal itself, and a push clears the record.

TestParseDismissArgs covers both --reason forms, a missing value, an empty reason, several IDs,
and an unknown flag — a typo like --resaon must fail rather than silently become a finding ID.

gofmt -l . clean, go vet ./... clean, go test ./... -count=1 green.

Local preflight returned 7 findings (all minor/trivial); all 7 are applied — target validation now
reuses repoPR so a non-positive PR fails as it does everywhere else, the post-dismiss assertion is
exact rather than decrease-only, and the docs distinguish narrowing a PR (which fixes a skipped
review's cause) from dismissing (which only records the decision).

Ref #42

Summary by CodeRabbit

  • New Features
    • Added crq dismiss <repo> <pr> <finding-id...> --reason to dismiss findings without GitHub review threads (idempotent and scoped to the current PR head).
    • Dismissed findings are excluded from subsequent crq next results for the same head, and a dismissed count is surfaced to help avoid deadlocks.
  • Documentation
    • Updated README and agent/loop guidance with the new thread-less dismissal workflow and rules.
  • Tests
    • Added coverage for CLI argument parsing and dismissal/deadlock edge cases.

crq resolve and crq decline both act on a review thread. A review-body
finding, a review-skipped notice and an outside-diff remark have none, so
neither command can touch them — and drain-first then blocks every future
round on a finding that can never drain. The observed end state was a PR
reporting that no review was ever requested for its current head, four
rounds running: the round could not start because the finding was
undrained, and the finding could not drain because nothing could act on
it.

crq dismiss <repo> <pr> <finding-id>... --reason "<why>" records that the
agent judged it. Dismissed findings are withheld from the action, and
crq next reports dismissed: N so nothing looks silently dropped.

Three choices worth stating. The reason is stored, not just demanded — a
dismissal that discards its justification is not auditable. Finding IDs
are content-derived rather than GitHub node IDs, so the repo and PR are
required to identify one. And the record is scoped to the round for the
current head: the same content yields the same ID, so a dismissal that
outlived its head would silently swallow the finding when the next
reviewer reports it again. Pushing supersedes the round and clears it,
which is the rule body findings already follow.

Dismissing enqueues the PR when crq is not yet tracking the current head,
because that is the deadlock's own signature — no round for the head at
all — and a dismissal with nowhere to live would change nothing.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dfb091cf-0086-4680-a4c3-a0f88fc70107

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds crq dismiss for threadless findings, stores dismissals per PR head, excludes dismissed findings from queue output, and documents the workflow for CLI and AI-agent usage.

Changes

Threadless Finding Dismissal

Layer / File(s) Summary
Round dismissal state
internal/state/state.go
Rounds persist dismissal reasons and provide methods to record and query dismissed finding IDs.
Dismissal service workflow
internal/crq/dismiss.go, internal/crq/service.go
Service.Dismiss validates findings, rejects threaded or unknown IDs, applies current-head checks, and records dismissal state.
Dismissal reporting and queue behavior
internal/crq/feedback.go, internal/crq/next.go, internal/crq/*_test.go
Current-head dismissed findings are removed from feedback and reported separately, with coverage for queue progress, idempotency, stale state, dry runs, and head scoping.
Queued review action control
internal/engine/next.go, internal/engine/next_test.go
Queued rounds permit pushes while reserved rounds remain held.
CLI command and usage guidance
cmd/crq/*, README.md, llms.txt, skills/coderabbit-queue/SKILL.md
The CLI parses and dispatches crq dismiss, while help and agent guidance describe handling findings without review threads.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Poem

A threadless finding hopped my way,
“Dismiss me kindly,” it did say.
With reason stored and head in view,
The queue knows what not to do.
Hop onward through review!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the new dismissal flow for findings GitHub cannot close, which is the main change in the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dismiss-unresolvable-finding

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a2d9880fa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/dismiss.go Outdated
Comment thread internal/crq/next.go Outdated
Comment thread internal/crq/dismiss.go Outdated
Three review findings, all correct.

Filtering happened in nextFromState, so a dismissal did nothing for crq
feedback or crq loop: convergence and the exit code are computed from the
full list, leaving the finding permanently actionable outside crq next.
The filter now lives in Feedback, before Converged, so there is one of it.

Nothing stopped dismissing a finding that HAS a thread. That would let a
round converge with the thread still open, skipping the resolve/decline
flow that puts the decision on the PR where the bot can answer it. A
threaded finding is now refused by name, and so is an ID that is not a
finding at this head at all — a stale copy-paste would otherwise record a
dismissal that silences whatever later matches it.

And the write is one CAS update again. Enqueueing first meant that a
failure between the enqueue and the record left a fire-eligible round
behind with nothing dismissed on it, which the autoreview daemon could
claim and spend a review on. Dismiss now reads the findings, validates
them, then creates or supersedes the round and records the decision in a
single write.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0d60a0ec2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/dismiss.go Outdated
Comment thread internal/crq/dismiss.go Outdated
Comment thread internal/crq/dismiss.go Outdated
Comment thread internal/crq/dismiss.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/crq/dismiss.go`:
- Around line 75-111: The Dismiss flow performs its CAS write directly in
dismiss.go instead of through service.go. Extract the s.store.Update logic into
a service.go effects-executor method such as recordDismissal, have Dismiss
invoke that method, and preserve the existing round, validation, and output
behavior while keeping DryRun write-free.
- Around line 58-108: The store.Update mutation currently reuses stale
feedback.Head and current data across CAS retries. Move feedback retrieval and
finding-map construction into the Update callback using the loaded round’s head,
refresh clean IDs against that feedback, and use the refreshed head for
NewRound/Supersede and validation so retries cannot regress state or reject
against stale findings.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 82b94e8d-e252-4104-98ce-f98ba0c342d5

📥 Commits

Reviewing files that changed from the base of the PR and between 03c7d2e and b0d60a0.

📒 Files selected for processing (10)
  • README.md
  • cmd/crq/main.go
  • cmd/crq/main_test.go
  • internal/crq/dismiss.go
  • internal/crq/feedback.go
  • internal/crq/next.go
  • internal/crq/next_test.go
  • internal/state/state.go
  • llms.txt
  • skills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Keep internal/crq limited to orchestration and effects wiring; service.go is the only effects executor for CAS writes and PostIssueComment, and DryRun must write nothing.

Files:

  • internal/crq/next_test.go
  • internal/crq/next.go
  • internal/crq/dismiss.go
  • internal/crq/feedback.go
internal/state/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/state/**/*.go: Persist schema v3 with one Round per PR, one global FireSlot, AccountQuota, and an Archive; never delete rounds, and preserve unknown JSON members during round trips.
Model round lifecycle transitions as Round methods, reject illegal edges, and never delete a round; completed rounds remain deduplication markers, while rate-limited retries use awaiting_retry without deleting fired history.

Files:

  • internal/state/state.go
🔇 Additional comments (9)
internal/state/state.go (1)

82-95: LGTM!

Also applies to: 622-640

internal/crq/next_test.go (1)

5-5: LGTM!

Also applies to: 306-424

internal/crq/feedback.go (1)

28-32: LGTM!

Also applies to: 435-448

internal/crq/next.go (1)

34-37: LGTM!

Also applies to: 222-225

cmd/crq/main.go (1)

214-241: LGTM!

Also applies to: 350-350, 373-374, 528-546, 788-811

cmd/crq/main_test.go (1)

115-152: LGTM!

README.md (1)

287-287: LGTM!

Also applies to: 388-388, 602-605

llms.txt (1)

51-51: LGTM!

Also applies to: 203-219

skills/coderabbit-queue/SKILL.md (1)

28-28: LGTM!

Also applies to: 172-193

Comment thread internal/crq/dismiss.go Outdated
Comment thread internal/crq/dismiss.go Outdated
Six review findings, all correct, and three of them P1.

A round tracking a different head is now refused rather than superseded.
If the head moved after Dismiss read the findings, superseding archives
the live round and points the queue back at a commit nobody is looking
at — and CAS retries cannot protect against a mutation that deliberately
overwrites newer state.

A round may only be CREATED when this call drains the head. Dismissing
one of several open findings used to queue a fire-eligible round that
DecideFire cannot hold back, because it never sees findings — so a pump
could spend the primary's quota on code the caller is still expected to
fix.

A finding carried from an older commit is refused. IDs hash the text, not
the commit, so recording one against the current head would silently
filter the identical finding when the current reviewer reports it.

And "no thread ID" turns out not to be the question. When the GraphQL
thread query fails, Feedback falls back to REST, which does not return
thread IDs at all, so an inline comment with an open thread arrives
looking threadless. Only sources that intrinsically cannot have one —
review bodies, prompt blocks, skip notices, issue comments — are
dismissible now.

The CAS write moves to service.go as recordDismissal, which is where
AGENTS.md says every write belongs, and it honours DryRun.
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c401ac3ded

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/dismiss.go
Comment thread internal/crq/feedback.go Outdated
Comment thread internal/crq/service.go Outdated
Three more findings, one P1.

The eligibility guard asked whether a round object existed, when what
matters is whether a FIRE-ELIGIBLE one would. A queued round is exactly
as dangerous as a newly created one: Pump can hand it to DecideFire,
which sees no findings and cannot enforce drain-first, so dismissing one
of several open findings could still buy a review of code the caller is
still fixing.

Repeating a dismissal failed on its own earlier success: Feedback filters
the dismissed ID out, so validating against the current findings alone
rejected it as unknown. The command is documented as idempotent and an
interrupted agent repeating itself is the ordinary case, so an ID this
round already dismissed is accepted and reported as such.

And the filter now checks the SOURCE, not just the ID. Finding IDs hash
the text, not where it came from, so a dismissed body finding later
delivered as an inline comment through the REST fallback hashes the same
— and an ID-only filter would hide a review thread that is open.
@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1f63aa558

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/service.go Outdated
Comment thread internal/crq/dismiss.go
Comment thread llms.txt Outdated
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Three findings, one P1 in the fix itself.

After a push the stored round is still on the PREVIOUS head, because
`crq next` returns on a current-head finding before it enqueues. Treating
that as a concurrent move rejected every dismissal and left the new head
in the exact drain-first deadlock this command exists to end. The two
cases are told apart by when the round was enqueued: after the findings
were read means somebody moved the PR forward and the decision is stale;
otherwise it is the ordinary post-push round, and it is superseded.

A finding another agent dismissed concurrently now counts as handled when
deciding whether other work is still open, instead of making this call
refuse over a finding that is already accounted for.

And the docs no longer send an agent to a command that will refuse it: a
`review_comment` finding lost its thread ID to the REST fallback, so it
is neither resolvable nor dismissible until crq can read review threads
again.
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5c2e86c24

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/service.go Outdated
Comment thread internal/crq/service.go
Comment thread README.md
Three findings, one P1 in the guard I added last round.

FireEligible answers "right now", which is the wrong question when
deciding whether a partial dismissal would leave a round able to buy a
review. A round cooling in awaiting_retry is not eligible this second and
becomes eligible the moment RetryAt passes, so letting the dismissal
through on that technicality just deferred the hazard to the cooldown
expiring. The guard asks whether the round will ever fire again.

State alone could not tell "no round for this head" from "the head moved
and nothing has enqueued it yet", so a push landing between Feedback and
the write could record the dismissal against the wrong commit. The head is
re-read once before recording.

And the README now carries the same caveat as the skill: a review_comment
finding lost its thread ID to the REST fallback and still has an open
thread, so it is neither resolvable nor dismissible until crq can read
threads again.
@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3da74ebde

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/dismiss.go
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
llms.txt (1)

203-206: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Threadless-finding enumeration omits issue_comment. Both docs list "a review body finding, a review-skipped notice, an outside-diff remark" as the things crq dismiss covers, but dismissibleSources in internal/crq/dismiss.go also marks issue_comment findings as dismissible — an agent reading either doc could wrongly assume an issue-comment finding must be handled some other way.

  • llms.txt#L203-L206: add "an issue-comment finding" to the enumerated list of threadless finding types.
  • skills/coderabbit-queue/SKILL.md#L174-L176: add the same to "Review-body findings, review-skipped notices and outside-diff remarks".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@llms.txt` around lines 203 - 206, The threadless-finding documentation must
also enumerate issue-comment findings as dismissible. Update llms.txt lines
203-206 and skills/coderabbit-queue/SKILL.md lines 174-176 to add “an
issue-comment finding” alongside the existing review-body, review-skipped, and
outside-diff types; no code change is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/crq/dismiss.go`:
- Around line 118-147: Short-circuit idempotent dismissal retries after
constructing wanted in internal/crq/dismiss.go lines 118-147: when len(wanted)
is zero, return the existing result with Already set to clean before calculating
remaining, re-reading the head, or calling recordDismissal. Make no code change
in internal/crq/service.go lines 399-407; future callers of recordDismissal must
preserve this no-op short-circuit discipline.

In `@internal/crq/service.go`:
- Around line 395-398: Update Service.recordDismissal so DryRun does not return
before loading state and applying the existing guard logic: compute the same
dismissed and already result classifications and surface the “other unaddressed
findings” rejection as a real run would, while skipping only CAS state writes
and preserving the returned results.

---

Outside diff comments:
In `@llms.txt`:
- Around line 203-206: The threadless-finding documentation must also enumerate
issue-comment findings as dismissible. Update llms.txt lines 203-206 and
skills/coderabbit-queue/SKILL.md lines 174-176 to add “an issue-comment finding”
alongside the existing review-body, review-skipped, and outside-diff types; no
code change is required.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9a763168-96a1-4247-8943-1eb040831be2

📥 Commits

Reviewing files that changed from the base of the PR and between b0d60a0 and 3fd635e.

📒 Files selected for processing (8)
  • README.md
  • internal/crq/dismiss.go
  • internal/crq/dismiss_test.go
  • internal/crq/feedback.go
  • internal/crq/next_test.go
  • internal/crq/service.go
  • llms.txt
  • skills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Keep internal/crq limited to orchestration and effects wiring; service.go is the only effects executor for CAS writes and PostIssueComment, and DryRun must write nothing.

Files:

  • internal/crq/dismiss_test.go
  • internal/crq/next_test.go
  • internal/crq/service.go
  • internal/crq/feedback.go
  • internal/crq/dismiss.go
internal/crq/service.go

📄 CodeRabbit inference engine (AGENTS.md)

The apply phase in service.go is the only place allowed to execute effects: CAS state writes and PostIssueComment; DryRun must report without writing.

Files:

  • internal/crq/service.go
🧠 Learnings (2)
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.

Applied to files:

  • internal/crq/dismiss_test.go
  • internal/crq/next_test.go
  • internal/crq/service.go
  • internal/crq/feedback.go
  • internal/crq/dismiss.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.

For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.

Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.

Applied to files:

  • internal/crq/dismiss_test.go
  • internal/crq/next_test.go
  • internal/crq/service.go
  • internal/crq/feedback.go
  • internal/crq/dismiss.go
🔇 Additional comments (10)
internal/crq/dismiss.go (2)

44-96: LGTM!


154-179: LGTM!

internal/crq/service.go (1)

594-607: LGTM!

internal/crq/feedback.go (1)

28-32: LGTM!

Also applies to: 435-452

internal/crq/next_test.go (1)

5-5: LGTM!

Also applies to: 376-384, 420-430

README.md (1)

285-292: LGTM!

Also applies to: 602-607

llms.txt (2)

51-51: LGTM!


220-283: LGTM!

skills/coderabbit-queue/SKILL.md (1)

178-189: LGTM!

Also applies to: 190-197

internal/crq/dismiss_test.go (1)

1-38: LGTM!

Comment thread internal/crq/dismiss.go
Comment thread internal/crq/service.go Outdated
A round left on the previous head can be past firing, so canStillFire said
no and the guard let a partial dismissal through — and the supersede below
then replaced it with a fresh queued round for the new head, which is exactly
the fire-eligible round the guard exists to prevent while findings are open.

Tell a stale round from a live one by Seq rather than by comparing its
EnqueuedAt against this host's clock: the fleet's hosts do not agree about
the time, and a worker whose clock lagged could supersede a newer round back
to a commit nobody is looking at.

A replay of a dismissal that already succeeded writes nothing, so it now
returns before the guard instead of being refused whenever some other finding
is still open — the command is documented as idempotent.

The dry run runs the same mutation against a throwaway copy of the state, so
it reports the ids it would record and the refusals it would hit instead of a
blanket success it cannot drift from.

Also clear the dismissal count when `crq next` finds the head moved (it is
head-scoped), and name issue-comment findings in both threadless-finding
lists, which dismissibleSources has covered all along.
@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The idempotence the short-circuit restores had no test that could fail:
the existing replay runs against a completed round, which the guard lets
through anyway. This one drives the case that broke — a dismissal that
ended the deadlock leaves a QUEUED round for the new head, a later review
reports something else, and the agent repeats the dismissal it does not
remember finishing — and asserts the replay is both accepted and a no-op.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1174458b5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/service.go
Comment thread internal/crq/feedback.go Outdated
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: ec3270b48f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

A round is no longer created only by an enqueue: `crq dismiss` creates one so
the decision has somewhere to live. `crq next` read any round at all as "a
review is running for this head", so the documented fix flow — fix locally,
dismiss the threadless finding, call again — was answered `hold`, telling the
caller to sit on the very fixes it had just made until a review of the code
they replace finished. That call then enqueued and pumped, so it could buy
that review itself.

A queued round has asked for nothing: no command posted, no quota spent, and a
push supersedes it for free. Only a round that actually got as far as reserving
the slot is a review worth holding a head for.

Also count the dismissals the round recorded rather than the current findings
that still match one. A finding ID hashes the text, so a bot editing or
deleting the comment a dismissal was made against left nothing to match and
`dismissed` fell back to zero — making a finding deliberately set aside read
exactly like one that was never reported.
@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

reviewRequested draws a line between queued and reserved, and only the
queued side is asserted. Pin the reserved one too, so a later widening of
the predicate releases a head with the command already about to be posted
and fails here instead of on a PR.
@kristofferR

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 589fd102d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/engine/next.go
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
skills/coderabbit-queue/SKILL.md (1)

28-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include the review_comment fallback exception here.

This row says every finding without .thread_id can be dismissed, but the detailed section correctly explains that source: "review_comment" may lack the ID only because GraphQL thread lookup failed and must be retried. Keep the loop instruction consistent so agents do not issue a rejected crq dismiss.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@skills/coderabbit-queue/SKILL.md` at line 28, Update the fix row in the
workflow table to exempt findings with source "review_comment" and no thread_id
from immediate crq dismiss; instruct agents to retry the GraphQL thread lookup
first, while retaining crq dismiss for genuinely threadless findings after
judgment.
internal/crq/dismiss.go (1)

162-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Dry-run log message will misreport a dismissal that never happened.

service.go's recordDismissal now runs its mutate closure against a throwaway state copy for DryRun and returns the real "would-be" dismissed IDs (previously it always returned []string{}, per the earlier "DryRun swallows the guard" fix). Since this log call is unconditional on s.cfg.DryRun, a dry run that would dismiss N findings will now log "...dismissed N finding(s)..." even though nothing was written to state — misleading anyone reading logs to debug a dry run.

🐛 Proposed fix
-	if s.log != nil && len(out.Dismissed) > 0 {
+	if s.log != nil && len(out.Dismissed) > 0 && !s.cfg.DryRun {
 		s.log.Printf("%s#%d dismissed %d finding(s) at %s: %s", repo, pr, len(out.Dismissed), out.Head, reason)
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/crq/dismiss.go` around lines 162 - 170, Update the dismissal log
guarded by s.log in the dismiss flow to distinguish dry runs from actual
dismissals: when s.cfg.DryRun is enabled, avoid reporting findings as
“dismissed” and use wording that indicates the findings would be dismissed;
preserve the existing message for non-dry-run executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/crq/service.go`:
- Around line 398-414: Move the round-phase fire-eligibility predicate
canStillFire from internal/crq/service.go:398-414 into
internal/engine/next.go:241-253 alongside reviewRequested, then update
Service.recordDismissal to call the engine predicate while preserving its
current behavior; add or extend engine table tests for the phase rules. The
internal/engine/next.go site is the consolidation target and requires the new
predicate, while internal/crq/service.go requires removal of its local
implementation and use of the engine version.

---

Outside diff comments:
In `@internal/crq/dismiss.go`:
- Around line 162-170: Update the dismissal log guarded by s.log in the dismiss
flow to distinguish dry runs from actual dismissals: when s.cfg.DryRun is
enabled, avoid reporting findings as “dismissed” and use wording that indicates
the findings would be dismissed; preserve the existing message for non-dry-run
executions.

In `@skills/coderabbit-queue/SKILL.md`:
- Line 28: Update the fix row in the workflow table to exempt findings with
source "review_comment" and no thread_id from immediate crq dismiss; instruct
agents to retry the GraphQL thread lookup first, while retaining crq dismiss for
genuinely threadless findings after judgment.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: af2c4355-507f-4950-95cb-1d929d3b3555

📥 Commits

Reviewing files that changed from the base of the PR and between 3fd635e and 589fd10.

📒 Files selected for processing (10)
  • internal/crq/dismiss.go
  • internal/crq/feedback.go
  • internal/crq/next.go
  • internal/crq/next_test.go
  • internal/crq/replay_test.go
  • internal/crq/service.go
  • internal/engine/next.go
  • internal/engine/next_test.go
  • llms.txt
  • skills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
internal/engine/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/engine/**/*.go: Keep internal/engine pure: decision logic must receive now, perform no I/O, context, or GitHub access, and table-test every rule.
DecideFire must be the single fire owner and evaluate guards in order: open, readable/current head, eligible phase, primary review unavailable, already reviewed, free slot, account quota, minimum interval, then adopt/post.
Keep convergence, fire, and co-reviewer rules in internal/engine; keep orchestration in internal/crq, and table-test engine rules.

Files:

  • internal/engine/next_test.go
  • internal/engine/next.go
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Keep internal/crq limited to orchestration and effects wiring; service.go is the only effects executor for CAS writes and PostIssueComment, and DryRun must write nothing.

Files:

  • internal/crq/feedback.go
  • internal/crq/replay_test.go
  • internal/crq/next.go
  • internal/crq/service.go
  • internal/crq/next_test.go
  • internal/crq/dismiss.go
internal/crq/service.go

📄 CodeRabbit inference engine (AGENTS.md)

The apply phase in service.go is the only place allowed to execute effects: CAS state writes and PostIssueComment; DryRun must report without writing.

Files:

  • internal/crq/service.go
🧠 Learnings (2)
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.

Applied to files:

  • internal/crq/feedback.go
  • internal/crq/replay_test.go
  • internal/crq/next.go
  • internal/crq/service.go
  • internal/crq/next_test.go
  • internal/crq/dismiss.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.

For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.

Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.

Applied to files:

  • internal/crq/feedback.go
  • internal/crq/replay_test.go
  • internal/crq/next.go
  • internal/crq/service.go
  • internal/crq/next_test.go
  • internal/crq/dismiss.go
🔇 Additional comments (8)
llms.txt (1)

51-51: LGTM!

Also applies to: 203-219

skills/coderabbit-queue/SKILL.md (1)

172-197: LGTM!

internal/crq/dismiss.go (1)

44-171: LGTM! Dismiss's validation ordering (read-before-write, idempotent short-circuit at len(wanted)==0, live head re-check via pullHead, delegation to recordDismissal) looks correct and matches the extensive test coverage in next_test.go.

internal/crq/feedback.go (1)

28-32: LGTM! The dismissed-count semantics (decisions recorded, not findings still matching) and the head-scoped, thread-aware filter are correctly implemented and match the documented intent and test coverage.

Also applies to: 435-457

internal/crq/next.go (1)

34-37: LGTM! report.Dismissed is correctly sourced from feedback.Dismissed and correctly reset to 0 when the head moves mid-decision, keeping the count properly head-scoped.

Also applies to: 114-126, 226-229

internal/crq/next_test.go (1)

312-445: LGTM! Excellent coverage of the deadlock-ending flow, idempotent replay under open work, partial-dismissal refusal, the queued-vs-reserved hold/push boundary, dismissed-count durability, and dry-run reporting-without-writing — all traced correctly against the production logic.

Also applies to: 453-506, 513-542, 544-591, 593-640, 642-686

internal/crq/replay_test.go (1)

196-207: LGTM!

internal/engine/next_test.go (1)

161-182: LGTM! Both new cases correctly pin the queued-vs-reserved boundary that reviewRequested implements.

Comment thread internal/crq/service.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant